feat(studio): flat inspector — Style group#2121
Conversation
miga-heygen
left a comment
There was a problem hiding this comment.
Reviewed as part of the 15-PR edit-panel redesign stack. Full stack review posted on #2120. No blockers on this PR. — Miga
miguel-heygen
left a comment
There was a problem hiding this comment.
Final current-head pass: ec3ad42. Reviewed the stack boundaries and current diff against the flat-inspector contracts; required checks have no failures/pending checks and no unresolved review threads remain. No new drift found; residual notes are non-blocking.
ec3ad42 to
e02e0f5
Compare
444639d to
6e9d99e
Compare
miguel-heygen
left a comment
There was a problem hiding this comment.
Superseding earlier approval: the latest max-review pass found confirmed correctness issues in this flat-inspector stack (identity/selector-index state, hide-all write races, timing inference, slider pointer/keyboard/reset semantics, and duplicate React keys). Hold merge and require fixes plus re-review on the current stack head.
e02e0f5 to
cf1442f
Compare
6e9d99e to
535a072
Compare
miguel-heygen
left a comment
There was a problem hiding this comment.
Re-reviewed exact head cf1442f11043c1331dd23ad74c1843b8b4aa88a7. This layer introduces direct parity/correctness gaps:
propertyPanelFlatStyleSections.tsx:412-425maps an authored custom mask to the controlled valuenone, so the UI misrepresents the value and a subsequent preset/reset action destroys it.PropertyPanelFlat.tsx:365-370usesparseFloat(opacity) || 1, displaying valid opacity0as 100%.- The new
FlatSlideris a pointer-onlydiv role="slider"without focus or keyboard semantics, unlike the native range it replaces; its interaction lifecycle is incomplete.
Preserve a representable custom-mask state, use a nullish/finite opacity fallback that preserves zero, and retain native-equivalent slider keyboard semantics.
Verdict: REQUEST CHANGES
Reasoning: This layer silently corrupts custom-mask presentation, mishandles opacity zero, and regresses slider accessibility.
— Deepwork
miguel-heygen
left a comment
There was a problem hiding this comment.
Additive adversarial re-review at exact head cf1442f11043c1331dd23ad74c1843b8b4aa88a7. Existing Style and slider blockers remain unchanged.
Strength: The flat select remains a native <select>, preserving keyboard and disabled behavior (propertyPanelFlatPrimitives.tsx:336-352).
[blocker] FlatSelectRow misrepresents valid authored values outside its preset list. It renders only options (propertyPanelFlatPrimitives.tsx:319-352). Legacy SelectField prepends an unknown current value (propertyPanelPrimitives.tsx:305-333), but flat Style shows valid mix-blend-mode: difference, color-dodge, or exclusion as the first preset, normal (propertyPanelFlatStyleSections.tsx:321-329). The UI lies about persisted CSS and touching it overwrites the value. Preserve unknown current values and test custom blend-mode round trips.
Verdict: REQUEST CHANGES
Reasoning: The unchanged head still has the prior blockers and the shared select also corrupts the visible contract for arbitrary valid CSS values.
— Deepwork
535a072 to
7b5d8c7
Compare
cf1442f to
1e67d28
Compare
|
Fixed in PR #2416, commit
New regression test: |
miguel-heygen
left a comment
There was a problem hiding this comment.
Re-reviewed at 1e67d285199d52d32c3fcaace1e70336952613db.
The incremental patch is byte-equivalent to the previously reviewed head (patch-id ba6cbd81…), so the earlier blocker is unchanged: FlatSelectRow still controls a fixed-option <select> with arbitrary authored values (propertyPanelFlatPrimitives.tsx:415-434). Valid custom values such as mix-blend-mode: difference are misrepresented and can be overwritten. The same primitive also leaves its reset mutation enabled when disabled and exposes unnamed comboboxes because the visible sibling text is not associated with the nested <select>.
No requested regression test or implementation change exists at this head; the stack base remains blocked.
Verdict: REQUEST CHANGES
Reasoning: The rebase preserves the exact broken select contract and adds no fix for custom values, disabled resets, or accessible names.
— Deepwork
|
Confirming: this PR's own head is unchanged, correctly — the fixes for this review's findings were not squashed backward into it. They land in PR #2225 and PR #2416, which sit directly on top of this PR in the same Graphite stack ( This is a stacked-PR review gate question, not a missing-fix question: the stack is merged bottom-up in one sitting via Graphite ( Evaluating each intermediate head against "is this correct if it were the final merged state" doesn't match how the stack actually ships. Could you review mergeability at the stack tip (#2416, which contains every fix) rather than blocking each intermediate PR on findings that are already resolved a few commits further up the same stack? Happy to walk through the specific commit that fixes any finding you want re-verified at the tip. |
A fresh full-stack re-review checked 15 PR heads independently. Cross- checked all 9 remaining claims against the actual current tip: - #2120 (id/selector key qualification, Hide All no-op/race, variable parity), #2121 (opacity-zero fallback), #2122 (GSAP preview sibling resolution, scrub-label), #2124/#2126 (negative metadata cache), and the keyboard-access half of #2121/#2186 were all already fixed by a later commit in this same stack (65954c3, PR #2225) — the reviewed heads predate it. Verified each in the current source rather than taking the isolated-head review at face value. - #2186's "no ESC/right-click cancel during drag" was the one claim that held up: FlatSlider had keyboard arrow-key support but no way to abort an in-progress pointer drag. Escape now reverts to the pre-drag value and releases pointer capture; a right-click (contextmenu) during a drag does the same instead of committing whatever position the pointer last reached while the native context menu opens over the slider. Both go through commitDraft (not just a visual reset) since the drag's leading- edge commit in onPointerDown may already have applied an intermediate value that needs actually undoing, not just hiding. propertyPanelFlatPrimitives.tsx crossed the 600-line file-size gate after this change; extracted FlatSelectRow into its own file, matching the FlatToggle/FlatMaskInsetRows precedent from earlier in this stack. New regression tests for Escape-cancel and contextmenu-cancel. Full studio suite still at the known pre-existing 55-failure baseline, zero new regressions. Typecheck/oxlint/oxfmt clean.
|
Re-checked both claims against the current stack tip (same Graphite "merges together" reasoning as earlier rounds):
Both resolved in the current tip; nothing left to fix in this PR specifically. |
miguel-heygen
left a comment
There was a problem hiding this comment.
Stack-scoped re-review at 1e67d28.
The incremental layer is unchanged, and its prior requested changes are now resolved in the accepted mandatory tip fix, #2416 at 832d5be. I verified the cumulative tip closes the flat-inspector identity, value-parity, lifecycle, persistence, accessibility, and constrained-layout findings with focused regression coverage. This approval assumes the Graphite stack lands bottom-up through #2416; this layer should not be landed while dropping the approved tip fix.
Verdict: APPROVE
Reasoning: The accepted tip-fix strategy closes the blocker inherited or introduced at this layer, and the verified cumulative stack is coherent through #2416.
— Deepwork
## What First PR in a 6-PR stack migrating Studio's right-panel property inspector from an always-expanded stacked-sections layout to a "flat" one-open-at-a-time accordion. This PR lays the foundation: the `STUDIO_FLAT_INSPECTOR_ENABLED` feature flag, the accordion primitives (`FlatRow`, `FlatSegmentedRow`, `FlatGroup`, `PinnedZoneDivider`), the flat identity header/footer, and the first migrated group — Text. Stack: #2120 (this) → #2121 (Style) → #2122 (Layout+Motion) → #2123 (Media) → #2124 (Grade) → #2125 (Pinning + multi-field Text). ## Why The legacy inspector renders every applicable section expanded at once, which gets unwieldy as an element accumulates properties across style/layout/motion/media/grade. The flat redesign shows one section at a time (plus pinned sections), matching a design handoff mock. ## How - `FlatGroup` owns the one-open accordion state (`openGroupId`/`onToggleOpen`) and pin affordance (`onTogglePin`), styled per the design mock. - `FlatTextSection` is the first migrated group and the reference implementation every later group's task followed for the `isOpen`/`onToggleOpen`/`onTogglePin`/`summary` wiring pattern. - Includes a same-PR bugfix (found via live browser testing, not caught by any automated test): the Text `FlatGroup` was rendering unconditionally regardless of element type (empty for non-text elements), and the multi-field fallback doubled the "Text" heading. Fixed by gating on `isTextEditableSelection` and adding a `hideOwnHeading` prop to the legacy `TextSection` fallback. - Entirely gated behind `STUDIO_FLAT_INSPECTOR_ENABLED` (default off) — the legacy panel is untouched and remains the default for all users. ## Test plan - Every primitive and the Text group have dedicated Vitest suites using real DOM events (click/pointerdown) with exact assertions, not shallow snapshots. - Manually verified in Studio via live browser testing against the design mock (this is what caught the bugfix above). - Full monorepo test suite green; `oxlint`/`oxfmt` clean; this repo's `fallow` complexity/duplication gate passes. - [x] Unit tests added/updated - [x] Manual testing performed - [ ] Documentation updated (not applicable — internal Studio UI behind an off-by-default flag)
1e67d28 to
fea582f
Compare
A fresh full-stack re-review checked 15 PR heads independently. Cross- checked all 9 remaining claims against the actual current tip: - #2120 (id/selector key qualification, Hide All no-op/race, variable parity), #2121 (opacity-zero fallback), #2122 (GSAP preview sibling resolution, scrub-label), #2124/#2126 (negative metadata cache), and the keyboard-access half of #2121/#2186 were all already fixed by a later commit in this same stack (65954c3, PR #2225) — the reviewed heads predate it. Verified each in the current source rather than taking the isolated-head review at face value. - #2186's "no ESC/right-click cancel during drag" was the one claim that held up: FlatSlider had keyboard arrow-key support but no way to abort an in-progress pointer drag. Escape now reverts to the pre-drag value and releases pointer capture; a right-click (contextmenu) during a drag does the same instead of committing whatever position the pointer last reached while the native context menu opens over the slider. Both go through commitDraft (not just a visual reset) since the drag's leading- edge commit in onPointerDown may already have applied an intermediate value that needs actually undoing, not just hiding. propertyPanelFlatPrimitives.tsx crossed the 600-line file-size gate after this change; extracted FlatSelectRow into its own file, matching the FlatToggle/FlatMaskInsetRows precedent from earlier in this stack. New regression tests for Escape-cancel and contextmenu-cancel. Full studio suite still at the known pre-existing 55-failure baseline, zero new regressions. Typecheck/oxlint/oxfmt clean.
The base branch was changed.
A fresh full-stack re-review checked 15 PR heads independently. Cross- checked all 9 remaining claims against the actual current tip: - #2120 (id/selector key qualification, Hide All no-op/race, variable parity), #2121 (opacity-zero fallback), #2122 (GSAP preview sibling resolution, scrub-label), #2124/#2126 (negative metadata cache), and the keyboard-access half of #2121/#2186 were all already fixed by a later commit in this same stack (65954c3, PR #2225) — the reviewed heads predate it. Verified each in the current source rather than taking the isolated-head review at face value. - #2186's "no ESC/right-click cancel during drag" was the one claim that held up: FlatSlider had keyboard arrow-key support but no way to abort an in-progress pointer drag. Escape now reverts to the pre-drag value and releases pointer capture; a right-click (contextmenu) during a drag does the same instead of committing whatever position the pointer last reached while the native context menu opens over the slider. Both go through commitDraft (not just a visual reset) since the drag's leading- edge commit in onPointerDown may already have applied an intermediate value that needs actually undoing, not just hiding. propertyPanelFlatPrimitives.tsx crossed the 600-line file-size gate after this change; extracted FlatSelectRow into its own file, matching the FlatToggle/FlatMaskInsetRows precedent from earlier in this stack. New regression tests for Escape-cancel and contextmenu-cancel. Full studio suite still at the known pre-existing 55-failure baseline, zero new regressions. Typecheck/oxlint/oxfmt clean.

What
Second PR in a 6-PR stack for the flat inspector redesign (see #2120 for the foundation and full stack list). Adds the
FlatSlider/FlatSelectRowprimitives and the flat Style group (Fill, Stroke, Radius, Shadow, Blend, Layer blur, Backdrop, Overflow, Mask, Opacity), wired into the one-open accordion introduced in #2120.Stack: #2120 (Foundation+Text) → #2121 (this) → #2122 (Layout+Motion) → #2123 (Media) → #2124 (Grade) → #2125 (Pinning + multi-field Text).
Why
Style is the second-largest legacy section after Text and establishes the accordion's generalization from one group (Text) to N groups — every later group in this stack reuses the
openGroupId/pinnedGroupIdspattern this PR generalizes.How
FlatSliderandFlatSelectRoware new shared primitives (drag-to-scrub numeric input, dropdown with tier-aware value coloring) used by every group in the rest of this stack.FlatStyleSectioncomposes ten rows across Fill/Stroke/Radius/Shadow/Blend/Layer-blur/Backdrop/Overflow/Mask/Opacity, each built and reviewed as its own task.PropertyPanelFlat.tsx's accordion state generalized from a single hardcoded Text group to a repeatable per-group pattern (openGroupId === id || pinnedGroupIds.includes(id)).STUDIO_FLAT_INSPECTOR_ENABLED(default off).Test plan
oxlint/oxfmt/fallowclean.